Drop es6 support in pursuit of supporting es8 #490

michaelkirk · 2023-07-10T22:32:38Z

Here's the reason for this change 🚀

I'd like Pelias to run on elasticsearch 8 some day. This is a step towards that.

Pelias does not support es6. Some things first deprecated in es6 and compatible with es7 are now breaking with my es8 tests.

This PR addresses those things as well as some other related cleanup I came across in the process.

Here's what actually got changed 👏

As suggested by @orangejulius, this PR is a subset of #488, including only the changes that are compatible with our existing elasticsearch library, which allows a more focused PR review.

Each commit is intended to address one change. I can break each commit up into separate PR's if you'd prefer.

This is a draft because it depends on the unreleased pelias/config#142
update: merged and rebased.

Here's how others can test the changes 👀

I've run the portland-metro example tests against a docker container built on top of this branch. Running it against more examples might be helpful.

I've manually run node scripts/list_analyzers.js and node scripts/output_mapping.js and the output looks reasonable, but I've never actually used those scripts for a purpose. If you are familiar with them, it'd be good to double check that they're giving you what you want.

It was introduced as part of "initial commit" without any particular explanation: ee26c2f I'm **guessing** this existed to facilitate updating es6 to es7. In es6 _type was recommended to be "_doc", and in es7, _doc was the only valid value. For details, see: https://www.elastic.co/guide/en/elasticsearch/reference/7.17/removal-of-types.html

Since we don't support es6, there's no need to specify it in any case, and removing it paves the way to es8. See: https://www.elastic.co/guide/en/elasticsearch/reference/7.17/removal-of-types.html

The rename to edge_ngram originally occurred in 6.4. Since the minimum supported elasticsearch is 7.4.2, we can safely use the modern spelling unconditionally.

From: https://www.elastic.co/guide/en/elasticsearch/reference/current/doc-values.html > Doc values are supported on almost all field types, with the notable exception of text and annotated_text fields. The error produced on elastic8.8 was: > [mapper_parsing_exception] unknown parameter [doc_values] on mapper [ngram] of type [text] AFAIK this was never supported, but maybe setting it to "false" was just silently ignored in older versions of elasticsearch?

I think they've been broken since b6e92d4, which changed schema to have a single mapping. Presumably this was done as part of deprecating `doc._type`

michaelkirk · 2024-02-12T18:10:05Z

Ok with pelias/config#142 merged, I've rebased and referenced the released config package, so this is ready for review.

michaelkirk · 2024-02-12T18:10:39Z

I've also just successfully re-run the portland-metro tests with this branch: https://github.com/michaelkirk-pelias/docker/tree/mkirk/drop-es6/projects/portland-metro

missinglink · 2024-02-12T18:11:36Z

Nice, the only things which caught my eye are renaming of edgeNGram and removal of doc_values.

I'll just double check the compatible versions tomorrow but otherwise it's pretty straightforward.

michaelkirk · 2024-02-12T18:21:37Z

In case you didn’t see it, I made some notes in the individual commits.

missinglink · 2024-02-13T12:27:35Z

Code review looks great 👍

I ran npm run integration against two versions in the 7.x family and it passed.

When I generated a new pelias/elasticsearch:8.12.1-beta image I wasn't able to connect to the instance to run the integration tests, the old elasticsearch JS client we use only supports the API up to version 7.x and also seems to lack support for the modern authentication method.

What was your intention for this PR, merging it now will drop support for 6.x but not add support for 8.x, was that your intent?

Would you consider making the elasticsearch client changes here too so that this lib supports both 7.x and 8.x before merging this and proceeding with other repos?

missinglink · 2024-02-13T12:30:00Z

I guess 110ac78 is what I'm missing?

orangejulius · 2024-02-13T12:52:32Z

Splitting out was at my request. Both just to make smaller easier to test, encapsulated PRs, and because doing the work of dropping old config options is very different than upgrading an NPM module.

And, honestly, because I still kinda don't love how the new Elasticsearch JS module does versioning, but I'm over that now 😆 and fortunately as 110ac78 shows the changes required are quite small.

I think this is probably good to merge, we'll just run a few other checks. The main thing is we have to make sure this PR generates a new major version of the schema module and is a breaking change. @michaelkirk in case you didn't figure it out (you figure out most things about Pelias 😁), we use semantic versioning (the concept) and semantic-release (the NPM package) to automatically generate changelogs and determine how package versions should from our git commit messages. It's pretty handy when it works and we get the syntax right.

For a major version change we need the "title" of the commit message to include something like feat(something): Change some part of this repo that is backwards incompatible. And then the "body" of the commit message needs to start with the text "BREAKING CHANGE:". It's a bit tricky, the main thing is to NOT use git commit -m so that git opens your editor window. The title is line one, line two is always an empty line, and line 3 is the start of the body.

This is mostly informational/for the future, as we can also put this in as maintainers when we merge a PR. It works in the commit body of the merge commit, and we can edit that in Github

michaelkirk · 2024-02-13T18:19:55Z

What was your intention for this PR, merging it now will drop support for 6.x but not add support for 8.x, was that your intent?

As @orangejulius alluded: Yes, that's the intention. Said differently, this PR is all the es8 changes that are backwards compatible with our legacy elasticsearch client.

I guess 110ac78 is what I'm missing?

There's a few more changes required for es8 - but they aren't compatible with our legacy elasticsearch client. We'll need everything in #488, which is a superset of this PR. My plan would be to rebase it after this gets merged. If you want me to go about it in a different way let me know. I know it is kind of a big burger to eat all at one time, so if you have ideas about how to better split it up let me know.

michaelkirk · 2024-02-13T18:24:18Z

semantic-release (the NPM package) to automatically generate changelogs

Thanks @orangejulius - I didn't know about semantic-release. Would you like me to amend the existing commit messages and force push or would you prefer I push another commit with that message format onto the branch?

missinglink · 2024-03-11T15:18:22Z

@michaelkirk can you please allow me to push changes to your branch so I can resolve this conflict and merge this PR 🙏 ?

michaelkirk · 2024-03-11T17:44:26Z

can you please allow me to push changes to your branch

Sorry I don't have that button.

I think this is because I forked the repositories into an organization (michaelkirk-pelias) rather than my personal account. The docs say (emphasis mine):

People with push access to the upstream repository of a fork owned by a personal account can commit to the forked branches.

At the time I thought having a namespaced organization for the several pelias repositories was a clever way to keep my personal namespace tidy. But as "clever" usually goes...

Anyway, I've merged main and repushed!

Feel free to do whatever is expedient in the future if you want to mangle any of my commits or branches. I don't have a lot of propriety for any of "my" commits making it in so long as the work moves forward.

michaelkirk mentioned this pull request Jul 11, 2023

Remove unused typeName which was deprecated in es7, forbidden in es8 pelias/config#142

Merged

michaelkirk force-pushed the mkirk/drop-es6 branch from 1264878 to c8ac51d Compare July 11, 2023 17:35

michaelkirk force-pushed the mkirk/drop-es6 branch from c8ac51d to f75abdb Compare July 21, 2023 23:40

michaelkirk mentioned this pull request Jul 22, 2023

Elasticsearch 8 support pelias/pelias#953

Open

6 tasks

michaelkirk force-pushed the mkirk/drop-es6 branch from f75abdb to 853b75c Compare February 9, 2024 20:42

michaelkirk added 7 commits February 9, 2024 12:52

Mapping types are deprecated in es7, error in es8

4979555

Since we don't support es6, there's no need to specify it in any case, and removing it paves the way to es8. See: https://www.elastic.co/guide/en/elasticsearch/reference/7.17/removal-of-types.html

Old 'edgeNGram' spelling dropped in elastic8

0ca7916

The rename to edge_ngram originally occurred in 6.4. Since the minimum supported elasticsearch is 7.4.2, we can safely use the modern spelling unconditionally.

inline plugin call since we only support es7+

b1ff960

Fix "list_analyzers" and "output_mapping"

431f3e6

I think they've been broken since b6e92d4, which changed schema to have a single mapping. Presumably this was done as part of deprecating `doc._type`

update to es8 compatible pelias-config

01d908c

michaelkirk force-pushed the mkirk/drop-es6 branch 6 times, most recently from 53b1e54 to 01d908c Compare February 9, 2024 21:58

michaelkirk marked this pull request as ready for review February 12, 2024 18:06

Merge remote-tracking branch 'upstream/master' into mkirk/drop-es6

3dab693

michaelkirk changed the title ~~Drop support es6 in pursuit of supporting es8~~ Drop es6 support in pursuit of supporting es8 Mar 11, 2024

missinglink merged commit 9fb43bc into pelias:master Mar 12, 2024

michaelkirk mentioned this pull request Mar 12, 2024

ES8 support - remove deprecated include_type_name parameter #483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop es6 support in pursuit of supporting es8 #490

Drop es6 support in pursuit of supporting es8 #490

michaelkirk commented Jul 10, 2023 •

edited

Loading

michaelkirk commented Feb 12, 2024

michaelkirk commented Feb 12, 2024

missinglink commented Feb 12, 2024

michaelkirk commented Feb 12, 2024 via email •

edited

Loading

missinglink commented Feb 13, 2024

missinglink commented Feb 13, 2024

orangejulius commented Feb 13, 2024 •

edited

Loading

michaelkirk commented Feb 13, 2024 •

edited

Loading

michaelkirk commented Feb 13, 2024

missinglink commented Mar 11, 2024 •

edited

Loading

michaelkirk commented Mar 11, 2024

Drop es6 support in pursuit of supporting es8 #490

Drop es6 support in pursuit of supporting es8 #490

Conversation

michaelkirk commented Jul 10, 2023 • edited Loading

Here's the reason for this change 🚀

Here's what actually got changed 👏

Here's how others can test the changes 👀

michaelkirk commented Feb 12, 2024

michaelkirk commented Feb 12, 2024

missinglink commented Feb 12, 2024

michaelkirk commented Feb 12, 2024 via email • edited Loading

missinglink commented Feb 13, 2024

missinglink commented Feb 13, 2024

orangejulius commented Feb 13, 2024 • edited Loading

michaelkirk commented Feb 13, 2024 • edited Loading

michaelkirk commented Feb 13, 2024

missinglink commented Mar 11, 2024 • edited Loading

michaelkirk commented Mar 11, 2024

michaelkirk commented Jul 10, 2023 •

edited

Loading

michaelkirk commented Feb 12, 2024 via email •

edited

Loading

orangejulius commented Feb 13, 2024 •

edited

Loading

michaelkirk commented Feb 13, 2024 •

edited

Loading

missinglink commented Mar 11, 2024 •

edited

Loading